PSR2/NamespaceDeclaration: do not enforce new line in inline HTML #815
Conversation
If a namespace declaration has a PHP close tag on the same line, the new line would be added as `T_INLINE_HTML` and therefore not be recognized as a blank line as the sniff solely looks for `T_WHITESPACE`. This then leads to a fixer conflict where the sniff just keeps adding new lines until it runs out of loops.
```
=> Fixing file: 0/6 violations remaining [made 46 passes]...
* fixed 0 violations, starting loop 47 *
PSR2.Namespaces.NamespaceDeclaration:81 replaced token 118 (T_INLINE_HTML on line 32) "\n\n" => "\n\n\n"
PSR2.Namespaces.NamespaceDeclaration:81 replaced token 153 (T_INLINE_HTML on line 58) "\n\n" => "\n\n\n"
=> Fixing file: 2/6 violations remaining [made 47 passes]...
* fixed 2 violations, starting loop 48 *
**** PSR2.Namespaces.NamespaceDeclaration:81 has possible conflict with another sniff on loop 46; caused by the following change ****
**** replaced token 118 (T_INLINE_HTML on line 32) "\n\n" => "\n\n\n" ****
**** ignoring all changes until next loop ****
=> Fixing file: 0/6 violations remaining [made 48 passes]...
* fixed 0 violations, starting loop 49 *
```
In my opinion, the sniff should bow out in that situation and should not enforce a new line as it may impact HTML display.
This commit implements this.
Includes a test safeguarding the fix.
|
FYI: Build failure is unrelated to this PR, but has to do with an outage at Coveralls. See lemurheavy/coveralls-public#1791 and https://status.coveralls.io/ |
There was a problem hiding this comment.
Thanks for raising this bug. I agree that the current behaviour is not correct. However, I think the problem is actually larger than described. I would prefer to see a solution which covers the wider problem, and not something so specific to the case being addressed here.
It strikes me that none of the existing tests have anything on the same line as the namespace statement. The sniff does not handle any of these "correctly" in my opinion. The following tests should be added to this sniff to safeguard its behaviour going forward:
namespace Vendor\Package; $var = true;
namespace Vendor\Package; // comment
namespace Vendor\Package; /* comment */
namespace Vendor\Package; ?> inline HTML <?php
namespace Vendor\Package;use Throwable; class Thing {}
namespace Vendor\Package;class Thing {}
namespace Vendor\Package;/* comment */
namespace Vendor\Package;?> inline HTML
This is not PHP.
<?php echo __FILE__;
namespace Vendor\Package
; $var = true;
$var = true;
namespace Vendor\Package; ?>
This is not PHP.
<?php
// This one fixes differently if it's at the end of the file (but it shouldn't).
namespace Vendor\Package; ?>Looking at PSR-2, the relevant quote for namespace statements is:
When present, there MUST be one blank line after the namespace declaration.
To me, this says that a namespace statement should be followed by exactly two newline characters, and the thing after that should not also be a newline character. I think if this sniff were to enforce that rule, the above test cases would fix as expected.
Following these rules, the last example in the above suggested tests would be fixed to:
namespace Vendor\Package;
?>which conforms to PSR-2 as far as I can tell. (It's not as elegant as <?php namespace Vendor\Package ?>, but it's compliant with the standard.)
If we want to make a special case for closing PHP tags, let's use a different error code so that consumers of this sniff can make decisions about ignoring (or not) that case.
|
@fredden Thanks for thinking about this. Unfortunately that is not how it works. We cannot interpret PSR's, we have to follow them to the letter. And there is nowhere in PSR2 where it spells out that no other code is allowed on the same line as a namespace declaration. So before we can change the behaviour of the sniff regarding this, whether or not other code (and/or comments) is allowed on the same line as a namespace declaration would need to be discussed in the PSR mailinglist or, more likely at present time: the PSR-PER repo. This will need to be clarified via the meta doc/addendum data for PSR-2 and only after that, would we be clear to make a change. To be honest, I'm perfectly happy for someone to go down that rabbit hole, but it won't be me and it is out of scope for this PR in my opinion. |
|
The rule says that "there MUST be one blank line after the namespace declaration." A "blank line" means two consecutive newline characters (otherwise the line between these two characters is not blank). "one blank line" means that three consecutive newline characters is not allowed (as that would be "two blank lines", not the prescribed "one"). I would argue that if another PHP statement were to be between the namespace declaration and its required blank line, then PSR-2 is not being respected, but I can see this is an interpretation. If we're going to go with an only-literal interpretation of the rule, then the sniff is broken differently. It should be searching for two consecutive newline characters in all the rest of the file (in HTML or between PHP statements or in a comment), and not complaining if any such cases are found anywhere in the file after the declaration. |
|
@fredden The way PSRs have been implemented in sniffs has always been strictly to the letter (including addendums) in this repo. This is not new. I'm just following Greg's lead in this and in the old repo, you can find numerous discussions demonstrating this. For both of your suggestions, the discussion should be had with the PSR people. Not here. |
|
Given the controversy around changing the behaviour of this sniff, let's focus on and fix the fixer conflict only. I think a8aaa01 might be enough to solve the fixer conflict. |
Description
If a namespace declaration has a PHP close tag on the same line, the new line would be added as
T_INLINE_HTMLand therefore not be recognized as a blank line as the sniff solely looks forT_WHITESPACE. This then leads to a fixer conflict where the sniff just keeps adding new lines until it runs out of loops.In my opinion, the sniff should bow out in that situation and should not enforce a new line as it may impact HTML display.
This commit implements this.
Includes a test safeguarding the fix.
Suggested changelog entry
PSR2.Namespaces.NamespaceDeclaration: prevent fixer conflict when the next thing after a namespace declaration is a PHP close tag
Related issues/external references
Related to #152
Types of changes